Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CICS connections breaking other ZE sessions #173

Merged
merged 12 commits into from
Jan 9, 2025

Conversation

AndrewTwydell
Copy link
Contributor

@AndrewTwydell AndrewTwydell commented Dec 17, 2024

What It Does

  • Changes method of pulling data from CMCI from Axios HTTP requests to the CICS SDK's getResource and getCache methods
    • Removes makeRequest method - refactored response handling
    • Refactored getPlexInfo method into smaller methods depending on what's provided in the profile
  • Removes the global rejectUnauthorized overrides which broke the other ZE sessions
  • Created toArray utility to replace all cases where we had to do a Array.isArray check

Review Checklist
I certify that I have:

Additional Comments
Resolves #159
Resolves #124

@codecov-commenter
Copy link

codecov-commenter commented Dec 17, 2024

Codecov Report

Attention: Patch coverage is 21.18227% with 160 lines in your changes missing coverage. Please review.

Project coverage is 28.20%. Comparing base (18afd7c) to head (a7c83f1).
Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
packages/vsce/src/utils/profileManagement.ts 20.68% 92 Missing ⚠️
...trees/CICSCombinedTrees/CICSCombinedProgramTree.ts 20.00% 12 Missing ⚠️
...es/vsce/src/trees/treeItems/CICSLibraryTreeItem.ts 27.27% 8 Missing ⚠️
.../CICSCombinedTrees/CICSCombinedTCPIPServiceTree.ts 25.00% 3 Missing ⚠️
...rc/trees/CICSCombinedTrees/CICSCombinedTaskTree.ts 25.00% 3 Missing ⚠️
.../trees/CICSCombinedTrees/CICSCombinedURIMapTree.ts 25.00% 3 Missing ⚠️
...es/CICSCombinedTrees/CICSCombinedWebServiceTree.ts 25.00% 3 Missing ⚠️
packages/vsce/src/trees/CICSRegionsContainer.ts 25.00% 3 Missing ⚠️
...trees/CICSCombinedTrees/CICSCombinedLibraryTree.ts 33.33% 2 Missing ⚠️
...ees/CICSCombinedTrees/CICSCombinedLocalFileTree.ts 33.33% 2 Missing ⚠️
... and 18 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #173      +/-   ##
==========================================
+ Coverage   27.73%   28.20%   +0.47%     
==========================================
  Files         147      147              
  Lines        5218     5113     -105     
  Branches      915      889      -26     
==========================================
- Hits         1447     1442       -5     
+ Misses       3771     3671     -100     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AndrewTwydell AndrewTwydell force-pushed the sdk-cache branch 2 times, most recently from 85531d9 to d3b74e1 Compare December 18, 2024 11:21
@AndrewTwydell AndrewTwydell force-pushed the fix-global-ru-overrides branch from 39b54e9 to 7e3de0a Compare December 18, 2024 12:02
Copy link
Contributor

@davenice davenice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of thoughts - look generally good though.

packages/vsce/src/trees/treeItems/CICSLibraryTreeItem.ts Outdated Show resolved Hide resolved
@AndrewTwydell AndrewTwydell force-pushed the fix-global-ru-overrides branch from 37980ec to 21eb530 Compare December 18, 2024 13:53
Base automatically changed from sdk-cache to main December 18, 2024 14:45
@AndrewTwydell AndrewTwydell force-pushed the fix-global-ru-overrides branch 2 times, most recently from e9be358 to 10de456 Compare December 18, 2024 18:48
@AndrewTwydell AndrewTwydell force-pushed the fix-global-ru-overrides branch 2 times, most recently from 2509799 to 9c7abfc Compare January 7, 2025 12:08
@@ -93,11 +89,12 @@ export class CICSRegionsContainer extends TreeItem {
* Count the number of total and active regions
* @param regionsArray
*/
private addRegionsUtility(regionsArray: [any]) {
private addRegionsUtility(regionsArray: any[]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[any] is an incorrect type (indicated an array with 1 item of type any)

@AndrewTwydell AndrewTwydell force-pushed the fix-global-ru-overrides branch 2 times, most recently from 999ab88 to 7614a24 Compare January 8, 2025 10:17
@AndrewTwydell AndrewTwydell marked this pull request as ready for review January 8, 2025 16:23
@AndrewTwydell
Copy link
Contributor Author

@zFernand0 I'd like your opinion on this as it resolves an issue you found relating to self-signed certs.

Majority of changes are in profileManagement.ts as it moves from Axios to the SDK which comes with changes in errors and response types. Lots of files are touched however because of the https import removal.

With the current lack of automated testing (WIP), I'm recruiting as many people as possible to manually test this (@davenice, @enamkhan, @chinmdas, amongst others) but the more the merrier!

I intend to squash my messy commits before merging 👍🏼

Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, LGTM! 😋

Thanks for fixing up all the typing issues. 🙏
And also for creating the common toArray function 🥳
Finally, thanks a lot for removing axios! 🙏

I know the primary purpose was to avoid messing with the reject unauthorized configuration of the https package, but I'm really happy with the little refactoring that happened here 🙏

.editorconfig Show resolved Hide resolved
@@ -99,7 +95,7 @@ export function getEnableTransactionCommand(tree: CICSTree, treeview: TreeView<a
});
}

async function enableTransaction(session: imperative.AbstractSession, parms: { name: string; regionName: string; cicsPlex: string }): Promise<ICMCIApiResponse> {
async function enableTransaction(session: imperative.AbstractSession, parms: { name: string; regionName: string; cicsPlex: string; }): Promise<ICMCIApiResponse> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this is a common interface...
{ name: string; regionName: string; cicsPlex: string; }

Curious if we can type it as one 😋


Also, I know I have a draft PR (#14) about migrating a lot of these SDK-like functions to the packages/sdk but I'm afraid that such PR might be a bit ambitious and also out of date 😋

I don't mind making some more updates and reducing the scope of it, if y'all would like 😋

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious if we can type it as one

Good plan, I'll add that onto this PR while we're already touching those methods.

Also, I know I have a draft PR (#14) about migrating a lot of these SDK-like functions to the packages/sdk but I'm afraid that such PR might be a bit ambitious and also out of date 😋

Now I'm more familiar with the monorepo I'll look over what you've got and comment over there. Seems silly to waste effort you've already done! 😎

@zFernand0
Copy link
Member

@zFernand0 I'd like your opinion on this as it resolves an issue you found relating to self-signed certs.

My apologies for taking so long, 😅
But I really like what's going on in this PR 🙏

I intend to squash my messy commits before merging 👍🏼

Similar to Zowe Explorer, I think we don't have to worry about constantly force-pushing to your own branch and instead, we can choose to squash PRs when merging (as Dave suggested on another PR conversation/thread).

Great work on this! 🙏

@zFernand0
Copy link
Member

With the current lack of automated testing (WIP), I'm recruiting as many people as possible to manually test this (@davenice, @enamkhan, @chinmdas, amongst others) but the more the merrier!

I'd love to see other people's thoughts/comments about their review/testing as well 🙏

@AndrewTwydell
Copy link
Contributor Author

Similar to Zowe Explorer, I think we don't have to worry about constantly force-pushing to your own branch and instead, we can choose to squash PRs when merging (as Dave suggested on another PR conversation/thread).

I'm a fan of a tidy commit history so a habit I've found myself in! More for me than anything else 😂

@davenice
Copy link
Contributor

davenice commented Jan 8, 2025

I've had a go with:

  • two separate hosts in play for z/OSMF, one with proper cert and one self-signed - both secured
  • cicsplexes on one of the hosts - one with security (incl self signed ssl) and one without security

I can access all of these systems at once without an issue.

i'm seeing a possible regression around what happens if you connect to a plex but specify a regionName - it seems to be displaying the "All Programs" and similar menus, and nesting everything as if it's a whole plex.

image

@AndrewTwydell
Copy link
Contributor Author

Good find @davenice! Will take a look now... Is that where you've specified a plex and region in the profile, not through any filtering

@davenice
Copy link
Contributor

davenice commented Jan 8, 2025

Good find @davenice! Will take a look now... Is that where you've specified a plex and region in the profile, not through any filtering

yes that's right - I think the issue is in profileManagement where we're deciding whether we've specified a region or group, the code is thinking we specified a group.

@AndrewTwydell AndrewTwydell force-pushed the fix-global-ru-overrides branch from 7614a24 to 1f3eb1e Compare January 8, 2025 22:31
@AndrewTwydell AndrewTwydell force-pushed the fix-global-ru-overrides branch from 1984e89 to 68d18e6 Compare January 8, 2025 22:38
@davenice
Copy link
Contributor

davenice commented Jan 8, 2025

OK - so I think we've resolved the issues we know about. I've tested with SMSS now (had forgotten that before).

Is there much else remaining to snag on this, @AndrewTwydell? I aspire to us getting this in this week ... if it's safe :-)

@AndrewTwydell
Copy link
Contributor Author

Nothing more that's been mentioned or found, and lots of manual testing has now been done. Seems I've got some fun conflicts to resolve later and we can hopefully get it in this afternoon after a final nod from the reviewers 🙂 Be very good to finally have this problem sorted!!!

@AndrewTwydell AndrewTwydell force-pushed the fix-global-ru-overrides branch from 68d18e6 to f770bb9 Compare January 9, 2025 13:54
@AndrewTwydell AndrewTwydell force-pushed the fix-global-ru-overrides branch 2 times, most recently from b6f5401 to 2dc70f0 Compare January 9, 2025 15:09
Co-authored-by: Dave Nice <[email protected]>
Signed-off-by: Andrew <[email protected]>
@AndrewTwydell AndrewTwydell force-pushed the fix-global-ru-overrides branch from 2dc70f0 to a7c83f1 Compare January 9, 2025 15:10
@davenice
Copy link
Contributor

davenice commented Jan 9, 2025

Hey @zFernand0 - @enamkhan, @AndrewTwydell and I have given this a bash along with another dev who was seeing some of the issues.

We're pretty confident that this is an improvement now (within the bounds that there isn't much automated testing).

We'd like to get this in - are you happy?

@zFernand0
Copy link
Member

We're pretty confident that this is an improvement now (within the bounds that there isn't much automated testing).

We'd like to get this in - are you happy?

100%
Thank you all for giving this a try and special shoutout to Andrew for the refactoring (and removing axios 🙏 )

@zFernand0 zFernand0 merged commit 106365f into main Jan 9, 2025
19 checks passed
@zFernand0 zFernand0 deleted the fix-global-ru-overrides branch January 9, 2025 15:27
@zFernand0 zFernand0 added the release-patch Indicates a patch to existing code has been applied label Jan 9, 2025
Copy link

github-actions bot commented Jan 9, 2025

Release succeeded for the main branch with some errors. ⚠️

The following packages have been published:

Powered by Octorelease 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-patch Indicates a patch to existing code has been applied released
Projects
Status: Closed
5 participants